feat(node-core): Add isolateTrace option to node-cron instrumentation
#18416
Conversation
sebws
changed the title
There was a problem hiding this comment.
Hey @sebws thanks for opening this PR and apologies for the late review!
This sounds reasonable to me in general but I would like us to also include an integration test. Primarily because this helps us understand the use case for isolateTrace in our automatic cron instrumentation. Would you mind adding a test? Ideally, one that failed before this change and passes with the change. We already have node-cron integration tests in our node-integration-tests and node-core-integration-tests packages. These should give you a good blueprint.
|
@Lms24 I added a test in |
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Thanks for adding the test! If you don't mind, yes, please also add it to node-integration-tests.
not sure if it would be preferred to be have this as an option at the individual level, e.g., override type of argArray so a full MonitorConfig could be passed into each .schedule().
I think the current approach is good enough for the time being. We can revisit if we need it in the args directly.
|
these tests fail if isolateTrace is set to false. just duplicated the previous test I had done |
Lms24
force-pushed
the
node-cron-trace-isolation
branch
from
8ef5f09 to
77dc9a3
Compare
|
FYI, I had to rebase your branch because there was a failing e2e test which had nothing to do with your changes. The fix was already merged to |
Lms24
changed the title
isolateTrace option to node-cron instrumentation
2 participants
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).not sure if it would be preferred to be have this as an option at the individual level, e.g., override type of
argArrayso a fullMonitorConfigcould be passed into each.schedule().however, it's also nice to have a default across all